Disallow duplicate dynamic_templates names
#51817
Conversation
Currently the same `dynamic_templates` name can be used more than once in the same mappings file. This most certainly is unintentional and can create issues like those reported in elastic#28988. Furthermore, when templates are matched later in `RootObjectMapper#findTemplate`, it looks like we simply pick the first matching one we see in the input array, so using the same template name twice sounds like an error prone idea anyway. This change checks for duplicate names on parsing and throws an error if we encounter the same name twice. Closes elastic#28988
cbuescher
added
>bug
:Search Foundations/Mapping
|
Pinging @elastic/es-search (:Search/Mapping) |
|
@jtibshirani I found this by accident since I was reviewing a PR in the same area earlier and thought this might be a simple fix. Let me know if this makes sense to you or if my thinking around the issue was a bit too shortsighted. |
| @@ -607,7 +607,7 @@ public void testDateDetectionInheritsFormat() throws Exception { | |||
| .endObject() | |||
| .endObject() | |||
| .startObject() | |||
| .startObject("dates") | |||
| .startObject("dates_with_explicit_format") | |||
There was a problem hiding this comment.
After having to change this test I'm not entirely sure anymore about the approach, in particular whether this test was lazily "missusing" a leniency in our code by using the name twice or wether this should work. In the end both templates seem to match different fields, so I'm not sure why the test used the same name?
There was a problem hiding this comment.
@cbuescher since commenting on #28988, I now think that some users are relying on this behavior (where dynamic templates are not disallowed/ deduplicated). For example, we recently found out in #29200 (comment) that APM creates multiple dynamic with the same name, but with different match_mapping_type values.
Would it make sense to discuss as a team to make sure we're okay with the direction? A note that if we do go this direction, I think we'd need a deprecation period before we start throwing an error, as there are users taking advantage of the leniency.
Absolutely, I wasn't aware of the other issue (#29200). From a first glance, it seems using the same dynamic template name there is problematic for similar reasons, but it makes sense to look at this from another angle again. We can close this PR for now and I will mark the original issue for discussion. |
Currently the same
dynamic_templatesname can be used more than once in thesame mappings file. This most certainly is unintentional and can create issues like
those reported in #28988. Furthermore, when templates are matched later in
RootObjectMapper#findTemplate, it looks like we simply pick the first matchingone we see in the input array, so using the same template name twice sounds like
an error prone idea anyway. This change checks for duplicate names on parsing
and throws an error if we encounter the same name twice.
Closes #28988